[jsweep] Clean update_pr_description_helpers.cjs#40584
Conversation
- Destructure detectionCaution directly from assembleMarkdownBodyParts result - Use template literal for contentWithCaution string building - Add 10 new tests (45 total, up from 35): - findIsland: exact startIndex/endIndex values, empty body, prefix content - buildAIFooter: historyUrl parameter, non-empty string assertion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40584 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold is 100). Affected files are skill scripts and an MCP server helper, not core business logic. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR performs a small cleanup in update_pr_description_helpers.cjs and expands unit tests around footer generation and island detection, aligning with the repo’s ongoing jsweep maintenance work.
Changes:
- Refactors CAUTION extraction to destructure
detectionCautionfromassembleMarkdownBodyParts()results. - Switches CAUTION content assembly to a template literal for readability.
- Adds new tests for
buildAIFooterandfindIslandto cover additional edge cases and assertions.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/update_pr_description_helpers.cjs | Minor refactor of CAUTION extraction/assembly without changing behavior. |
| actions/setup/js/update_pr_description_helpers.test.cjs | Adds coverage for buildAIFooter history URL propagation and additional findIsland index edge cases. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — commenting with two non-blocking observations on test quality.
📋 Key Themes & Highlights
Key Themes
- Untested truthy branch:
updateBody'sdetectionCautioninjection path (the code actually touched by this PR) has no test exercising a non-empty caution value — this gap pre-dates the PR but is worth closing. - Redundant baseline test:
should return a non-empty footer stringcheckstypeof === 'string'andlength > 0, both already implied by the richertoContaintests above it.
Positive Highlights
- ✅ Destructuring over chained property access is idiomatic and cleaner.
- ✅ Template literal replaces string concatenation consistently with the rest of the file.
- ✅ New
findIslandtests are precise and well-chosen: boundary (startIndex=0), empty body (-1indices), and prefix-offset correctness all fill real gaps. - ✅
historyUrloptional parameter propagation now has an explicit test. - ✅ All 45 tests pass with format/lint/typecheck green.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 66.3 AIC · ⌖ 7.45 AIC · ⊞ 6.9K
|
|
||
| it("should return a non-empty footer string", () => { | ||
| const footer = buildAIFooter("Test Workflow", "https://github.com/owner/repo/actions/runs/123"); | ||
| expect(typeof footer).toBe("string"); |
There was a problem hiding this comment.
[/tdd] The typeof footer === 'string' assertion is redundant — earlier tests already imply the return type via .toContain(), and the JSDoc @returns {string} documents the contract.
💡 Consider a behavioural assertion instead
A more valuable spec would verify something the other tests don't cover, for example that a footer produced with no optional args still contains the mandatory attribution marker. The current check reads as a type-level sanity probe rather than a specification.
| }).detectionCaution; | ||
| const contentWithCaution = detectionCaution ? detectionCaution + "\n\n" + sanitizedNewContent : sanitizedNewContent; | ||
| }); | ||
| const contentWithCaution = detectionCaution ? `${detectionCaution}\n\n${sanitizedNewContent}` : sanitizedNewContent; |
There was a problem hiding this comment.
[/tdd] The detectionCaution truthy branch — ${detectionCaution}\n\n${sanitizedNewContent} — has no test coverage. The PR adds tests for findIsland and buildAIFooter, but the code path that was actually changed (updateBody's caution injection) is never exercised with a non-empty detectionCaution.
💡 Suggested test scaffold
To cover this branch you would need to mock assembleMarkdownBodyParts to return a non-empty detectionCaution, then assert the output starts with the caution block followed by the sanitized content:
it('should prepend detectionCaution before content when threat is detected', () => {
// mock markdown_body_helpers to return detectionCaution = '> [!CAUTION]\n> ...'
const result = updateBody({ ...baseParams, operation: 'replace' });
expect(result.indexOf(caution)).toBeLessThan(result.indexOf('New content'));
});Without this test the template-literal on this line is validated only by visual inspection.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (5 tests analyzed)
Go: 0 (
|
There was a problem hiding this comment.
The production changes are correct — both the destructuring refactor and template literal substitution are semantically equivalent to the original code, with no behavioral difference.
Review summary
Production code (update_pr_description_helpers.cjs)
Both changes are pure style refactors:
- Destructuring
{ detectionCaution }fromassembleMarkdownBodyParts(...)is exactly equivalent to chaining.detectionCautionon the result. - Template literal
`${detectionCaution}\n\n${sanitizedNewContent}`is exactly equivalent to the string concatenation it replaces.
No logic change, no new code path, no new risk.
Tests (update_pr_description_helpers.test.cjs)
Four of the five new tests are solid:
historyUrlpropagation throughbuildAIFooter— correctly validates the third argument is forwarded.findIslandboundary tests (full-body island, empty body, prefix offset) — these pin exact index semantics against the realindexOf-based implementation, which is exactly the right approach for a function whose return values are used insubstring()calls.
One non-blocking observation is left inline on the fifth test.
🔎 Code quality review by PR Code Quality Reviewer · 98.8 AIC · ⌖ 7.21 AIC · ⊞ 5.1K
| expect(footer).toContain(historyUrl); | ||
| }); | ||
|
|
||
| it("should return a non-empty footer string", () => { |
There was a problem hiding this comment.
Redundant test: this it block adds no regression protection beyond what adjacent tests already provide.
💡 Details
Every other buildAIFooter test (e.g. line 22, which asserts toContain("Test Workflow") and toContain(runUrl)) already implicitly proves the function returns a non-empty string. If buildAIFooter broke and returned undefined or "", those tests would fail first.
The expect(typeof footer).toBe("string") assertion is a no-op: TypeScript enforces this at compile time, and if it somehow weren't a string at runtime, footer.length would throw or return undefined, failing toBeGreaterThan(0) on its own.
Consider replacing this with a more targeted assertion — for example verifying the footer contains the <!-- gh-aw-agentic-workflow: ... --> XML marker or the Generated by prefix — so this slot in the suite actually catches a novel failure mode.
|
@copilot please summarize the remaining blockers and next step so this PR can get back to review.
|
|
@copilot review all comments and address unresolved review feedback, starting with the redundant footer assertion.
|
|
Please summarize the remaining blockers and the next fix after the review comments are handled.
|
|
@copilot refresh the branch and rerun checks so the unstable merge state can clear.
|
|
``
|
|
``
|
Summary
Minor code-quality sweep on
update_pr_description_helpers.cjs: replaces a method-chained property access with object destructuring and converts a string concatenation to a template literal. Companion test additions expand coverage forbuildAIFooterandfindIsland.Changes
actions/setup/js/update_pr_description_helpers.cjs— refactor (low impact)detectionCautionis now extracted via object destructuring instead of a chained property access, improving readability.actions/setup/js/update_pr_description_helpers.test.cjs— tests (low impact)buildAIFooter: new cases cover history-URL inclusion and guaranteed non-empty output.findIsland: new cases cover full-body span match, empty-body input, and prefix-content edge cases.Risk assessment
Commit trail
7772a4ab0jsweep: clean update_pr_description_helpers.cjsdc0ceb209maininto branch47b5ac7a5maininto branch